Skip to content

Boot the app asynchronously - #172

Merged
superkhau merged 1 commit into
masterfrom
feature/boot-app-asynchronously
Feb 24, 2015
Merged

Boot the app asynchronously#172
superkhau merged 1 commit into
masterfrom
feature/boot-app-asynchronously

Conversation

@superkhau

@superkhau superkhau commented Dec 2, 2014 •
edited
Loading

Copy link
Copy Markdown
Contributor

Connected to #170

@bajtos bajtos self-assigned this Dec 2, 2014
@bajtos

bajtos commented Dec 2, 2014

Copy link
Copy Markdown
Member

Please rebase on top of the current master, I have landed a change of server/server.js recently.

@bajtos

bajtos commented Dec 2, 2014

Copy link
Copy Markdown
Member

I find it weird that app.start is defined from within a boot callback. Please move the definition to the top level, before you call boot(app).

@bajtos

bajtos commented Dec 2, 2014

Copy link
Copy Markdown
Member

BTW this PR is only a part of 170, there are three tasks in that issue. I have modified PR description to prevent auto-close.

@superkhau
superkhau force-pushed the feature/boot-app-asynchronously branch 3 times, most recently from 4cd806e to 32ec142 Compare December 3, 2014 02:00
bajtos
bajtos reviewed Dec 3, 2014
View reviewed changes
Comment thread templates/components/api-server/template/server/server.js Outdated

@bajtos bajtos Dec 3, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be implemented by loopback-boot, please remove L19-L20.

It would be nice to add a unit-test for this too, see https://github.com/strongloop/loopback-workspace/blob/master/test/end-to-end.js.

The test can do this for example:

  • scaffold a new app
  • add async boot script
  • require server/server.js
  • check that there is a flag indicating async boot process and that the event ready is eventually emitted

@superkhau
superkhau force-pushed the feature/boot-app-asynchronously branch from 32ec142 to 27c0893 Compare December 3, 2014 08:36
@bajtos

bajtos commented Dec 3, 2014

Copy link
Copy Markdown
Member

The new version of server/server.js looks good to me.

As I mentioned in a line comment, I'd like to see unit tests before landing this patch.

see https://github.com/strongloop/loopback-workspace/blob/master/test/end-to-end.js.
The test can do this for example:

  • scaffold a new app
  • add async boot script
  • require server/server.js
  • check that there is a flag indicating async boot process and that the event ready is eventually emitted

@bajtos bajtos assigned superkhau and unassigned bajtos Dec 5, 2014
@bajtos bajtos added #wip and removed #review labels Dec 8, 2014
@superkhau superkhau added #sprint62 and removed #wip labels Jan 13, 2015
@bajtos bajtos mentioned this pull request Jan 15, 2015
Closed
@ghost ghost added #plan and removed #sprint62 labels Jan 26, 2015
@superkhau superkhau added #fib-3 and removed #plan labels Jan 27, 2015
@bajtos bajtos mentioned this pull request Feb 6, 2015
Closed
3 tasks
@superkhau
superkhau force-pushed the feature/boot-app-asynchronously branch 2 times, most recently from 5f137ee to 84dce50 Compare February 20, 2015 02:45
@superkhau superkhau changed the title Update boot() to serve the app asynchronously Asynchronous boot scripts Feb 20, 2015
@superkhau superkhau changed the title Asynchronous boot scripts Async boot scripts Feb 20, 2015
@superkhau

superkhau commented Feb 20, 2015

Copy link
Copy Markdown
Contributor Author

@bajtos Review please.

@superkhau superkhau assigned bajtos and unassigned superkhau Feb 20, 2015
@superkhau superkhau changed the title Async boot scripts Boot app asynchronously Feb 20, 2015
bajtos
bajtos reviewed Feb 20, 2015
View reviewed changes
Comment thread templates/components/api-server/template/server/server.js Outdated

@bajtos bajtos Feb 20, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must preserve the event started, it's used by server/boot/explorer.js to print a message with Explorer URL, but only after the app was started.

@superkhau superkhau Feb 20, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. It's different from booted, adding back.

@bajtos bajtos assigned superkhau and unassigned bajtos Feb 20, 2015
@superkhau
superkhau force-pushed the feature/boot-app-asynchronously branch 2 times, most recently from d76fffe to e886ce4 Compare February 21, 2015 02:01
@superkhau superkhau changed the title Boot app asynchronously Boot the app asynchronously Feb 21, 2015
@superkhau

superkhau commented Feb 21, 2015

Copy link
Copy Markdown
Contributor Author

@slnode test please

@superkhau

superkhau commented Feb 21, 2015

Copy link
Copy Markdown
Contributor Author

@bajtos Review please.

@superkhau superkhau assigned bajtos and unassigned superkhau Feb 21, 2015
@bajtos

bajtos commented Feb 24, 2015

Copy link
Copy Markdown
Member

One more thing to fix before you can merge this patch: The scaffolded version of loopback-boot (2.4.0 ATM - source) does not support async boot scripts. Please bump up the version to ^2.6.5.

Note that the version of loopback-boot in the toplevel package.json must remain at 1.x, that's a completely different thing.

Other than that, the change LGTM. You can land it without further review after you upgrade loopback-boot.

@bajtos bajtos assigned superkhau and unassigned bajtos Feb 24, 2015
@bajtos

bajtos commented Feb 24, 2015

Copy link
Copy Markdown
Member

Also please make sure the test is passing on Jenkins before merging.

@bajtos bajtos mentioned this pull request Feb 24, 2015
Merged
@superkhau
superkhau force-pushed the feature/boot-app-asynchronously branch from e886ce4 to dd470e9 Compare February 24, 2015 21:06
@superkhau
superkhau force-pushed the feature/boot-app-asynchronously branch from dd470e9 to ab51347 Compare February 24, 2015 21:08
superkhau added a commit that referenced this pull request Feb 24, 2015
@superkhau
Merge pull request #172 from strongloop/feature/boot-app-asynchronously
2fc2d1f 
Boot the app asynchronously
@superkhau
superkhau merged commit 2fc2d1f into master Feb 24, 2015
@superkhau
superkhau deleted the feature/boot-app-asynchronously branch February 24, 2015 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@superkhau superkhau

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@superkhau @bajtos